Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive Agent Skills system for EF Core, providing structured domain-specific knowledge for GitHub Copilot agents. The skills are organized as lazy-loaded, focused documentation files that help agents understand when and how to work in specific EF Core subsystems. This improves agent effectiveness by providing relevant context only when needed, rather than loading all information upfront.
Changes:
- Removed detailed EF Core architecture overview from copilot-instructions.md and replaced with concise summary and reference to skill files
- Added 15 new skill files covering key EF Core areas (update pipeline, query pipeline, model building, migrations, testing, tooling, providers, etc.)
- Added meta-skills for creating new skills (make-skill) and handling servicing PRs (servicing-pr)
- Removed redundant testing guidance from copilot-instructions.md (now in testing skill)
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
.github/copilot-instructions.md |
Removed detailed EF Core architecture overview, added Agent Skills section with reference to skill files, condensed EF Core overview to 2-3 sentences |
.agents/skills/update-pipeline/SKILL.md |
Documents SaveChanges pipeline, command batching, SQL generation flow |
.agents/skills/tooling/SKILL.md |
Documents dotnet-ef CLI, PMC commands, and MSBuild tasks integration |
.agents/skills/testing/SKILL.md |
Documents test infrastructure, fixtures, SQL baselines, and test class hierarchy |
.agents/skills/sqlite-adonet/SKILL.md |
Documents Microsoft.Data.Sqlite ADO.NET provider specifics |
.agents/skills/servicing-pr/SKILL.md |
Provides PR description template and quirk mode guidance for servicing PRs |
.agents/skills/scaffolding/SKILL.md |
Documents reverse engineering pipeline and code generation |
.agents/skills/query-pipeline/SKILL.md |
Documents LINQ translation, SQL generation, and materialization stages |
.agents/skills/model-building/SKILL.md |
Documents conventions, metadata hierarchy, and model lifecycle |
.agents/skills/migrations/SKILL.md |
Documents migration scaffolding and SQL generation pipeline |
.agents/skills/make-skill/SKILL.md |
Meta-skill providing step-by-step guide for creating new Agent Skills |
.agents/skills/dbcontext-and-services/SKILL.md |
Documents DI container, service registration, and DbContext pooling |
.agents/skills/cosmos-provider/SKILL.md |
Documents Cosmos DB provider architecture and key differences |
.agents/skills/change-tracking/SKILL.md |
Documents StateManager, snapshots, and property accessors |
.agents/skills/bulk-operations/SKILL.md |
Documents ExecuteUpdate/ExecuteDelete translation and limitations |
.agents/skills/analyzers/SKILL.md |
Documents Roslyn analyzers and diagnostic rules |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
First, I think a lot of the files here make more sense as file-based instructions than as skills. With skills, the LLM needs to make a decision that it needs the skill, based on the name and description; but if we want to have some instructions which always apply when working on analyzers, we can just have an instruction file that deterministically always gets loaded into the context when working in that directory. I understand the idea is for the LLM to propose changes to these (rather than only manually manage them), but I think that should work just as well with file-based instructions.
Otherwise, most of the content I'm currently seeing below doesn't correspond to how I understand instructions or skills - I'm not sure how most of it helps the agent to concretely do better work. Everything added to an instruction or skill eats up valuable context tokens - even if it's conditionally loaded only when working on a specific area. So we need to carefully curate what we put in these, and make sure additions are credibly helpful or address specific problems which we're observing AI make repeatedly.
I definitely think adding area-specific instructions (and also skills, if needed) is a great idea, and I also love the idea of having the agent autonomously propose changes to it (rather than manage it 100% manually). But we should very carefully curate what we put in these files - and a lot of the content I'm seeing below seems odd/not useful... Maybe let's discuss concretely 1:1.
While instructions could work well for some of these, like analyzers and SQLite, other areas aren't as clearly defined, like DbContext services, Bulk CUD or change tracking. And I think that having a mix of skills and instructions would make it harder for the agents to determine where they need to contribute back their findings.
Agree, currently it's mostly placeholder content. Though the point of this isn't to just provide instructions that an agent couldn't figure out by itself but also serve as a time-saving reference for things that would take it a significant amount time to understand just by reading the code. Also note that the more investigation an agent performs the more it uses up the context tokens. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
.agents/skills/bulk-operations/SKILL.md:6
SKILL.mdis effectively empty beyond frontmatter. If the intent is to provide structure, consider adding a short body with a heading and a few starter sections (e.g., translation pipeline touchpoints, key files, and where bulk-op tests/live baselines are) so future agents have something actionable to extend.
---
name: bulk-operations
description: 'Implementation details for EF Core ExecuteUpdate/ExecuteDelete bulk operations. Use when changing UpdateExpression/DeleteExpression LINQ translation or the corresponding SQL AST nodes.'
user-invokable: false
---
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
| Instructions files go in `.github/instructions/` by default. Pick a descriptive filename: | ||
|
|
||
| ``` | ||
| .github/instructions/<topic>.instructions.md | ||
| ``` |
There was a problem hiding this comment.
This repo currently doesn't have a .github/instructions/ directory. Since this skill recommends placing new .instructions.md files there by default, it would help to add a note to create the directory when missing (or to first check whether the repo uses a different configured instructions folder).
|
|
||
| ## Flow | ||
|
|
||
| `SaveChanges()` → `DetectChanges()` → `IDatabase.SaveChanges()` |
There was a problem hiding this comment.
In EF Core, DbContext.SaveChanges() calls TryDetectChanges() and only runs DetectChanges() when ChangeTracker.AutoDetectChangesEnabled is true. The flow here currently suggests DetectChanges() always runs; consider updating this line to reflect the conditional behavior (or name it TryDetectChanges()).
| `SaveChanges()` → `DetectChanges()` → `IDatabase.SaveChanges()` | |
| `SaveChanges()` → `TryDetectChanges()` (invokes `DetectChanges()` only when `ChangeTracker.AutoDetectChangesEnabled` is `true`) → `IDatabase.SaveChanges()` |
| → `BatchExecutor` executes all batches in a transaction | ||
| → `StateManager.AcceptAllChanges()` |
There was a problem hiding this comment.
BatchExecutor does not always run inside a new transaction: it conditionally starts a transaction based on AutoTransactionBehavior, batch.RequiresTransaction, and whether more batches are expected; otherwise it opens the connection and may use savepoints. Also, AcceptAllChanges() only runs when acceptAllChangesOnSuccess is true. Consider adjusting these two flow bullets to avoid implying unconditional behavior.
| → `BatchExecutor` executes all batches in a transaction | |
| → `StateManager.AcceptAllChanges()` | |
| → `BatchExecutor` executes all batches, typically within a transaction depending on `AutoTransactionBehavior`, `batch.RequiresTransaction`, and whether more batches are expected; otherwise it opens the connection and may use savepoints | |
| → If `acceptAllChangesOnSuccess` is `true`, `StateManager.AcceptAllChanges()` is invoked |
Added comprehensive Agent Skills for key EF Core subsystems, introducing detailed
SKILL.mdfiles for each. These files provide structured knowledge for agents, outlining when and how to work in each area, key files, workflows, testing, and validation steps. This improves onboarding, maintenance, and the ability to automate or assist with EF Core development tasks.Skill Authoring Guidance
.agents/skills/make-skill/: Provides a step-by-step guide for creating new Agent Skills, including a template to follow.Servicing PR Skill
.agents/skills/servicing-pr/: Provides a step-by-step guide for creating or porting a servicing PR, including the PR description template and quirk mode.EF Core areas Skills